feat: make PlayerInfoPanel available in replays - #3855
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe client now detects spectator and replay state, opens read-only player panels for spectators, disables spectator actions, and guards radial menu updates when no active player exists. ChangesSpectator mode cohort
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to When player context becomes unavailable, the radial menu can remain open instead of closing, leaving replay or spectator UI in an inconsistent state. The PR is otherwise mergeable, but this bounded UI issue needs a targeted fix or explicit owner awareness. Sequence Diagram(s)sequenceDiagram
participant User
participant MainRadialMenu
participant GameView
participant PlayerPanel
User->>MainRadialMenu: right-click player tile
MainRadialMenu->>GameView: isSpectator()
GameView-->>MainRadialMenu: spectator status
MainRadialMenu->>PlayerPanel: open read-only panel
PlayerPanel->>PlayerPanel: render selected player data
PlayerPanel-->>User: show read-only player view
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
954-990:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock intent-capable modals in replay mode as well
Line 954 and Line 975 still allow modal rendering in replay when state is already set. Those modals can emit intent events, so replay is not fully read-only unless these branches are also gated by replay mode.
Suggested patch
+ const canInteract = !isReplay; ... - ${this.sendTarget + ${canInteract && this.sendTarget ? html` <send-resource-modal ... - ${this.moderationTarget + ${canInteract && this.moderationTarget ? html` <player-moderation-modal🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/graphics/layers/PlayerPanel.ts` around lines 954 - 990, PlayerPanel still renders intent-capable modals when sendTarget or moderationTarget are set even in replay mode; update the template branches that render <send-resource-modal> and <player-moderation-modal> to also check the replay flag and skip rendering when in replay. Locate the modal conditional expressions referencing this.sendTarget (and this.sendMode) and this.moderationTarget inside PlayerPanel and add a guard such as && !this.isReplay (or the project's replay flag, e.g., this.g.isReplay) so the modals are not instantiated in replay/read-only mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/graphics/layers/PlayerPanel.ts`:
- Around line 954-990: PlayerPanel still renders intent-capable modals when
sendTarget or moderationTarget are set even in replay mode; update the template
branches that render <send-resource-modal> and <player-moderation-modal> to also
check the replay flag and skip rendering when in replay. Locate the modal
conditional expressions referencing this.sendTarget (and this.sendMode) and
this.moderationTarget inside PlayerPanel and add a guard such as &&
!this.isReplay (or the project's replay flag, e.g., this.g.isReplay) so the
modals are not instantiated in replay/read-only mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38b69f60-1712-45f5-a37b-168a4f44109a
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts
|
Please test it locally by running npm run dev:prod You can watch all prod replays that way And a screenshot would be nice then |
|
And prettier is failing, run |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/graphics/layers/MainRadialMenu.ts`:
- Around line 118-126: The issue is that this.clickedTile is accessed inside
async callbacks in the methods around lines 118 and 188, leading to stale or
incorrect tile usage if it changes before the promise resolves. Fix this by
capturing the current clickedTile value into a local constant before calling
myPlayer.actions or other async functions, then use that local constant inside
the callback to update the player actions consistently for the intended tile.
In `@src/client/graphics/layers/PlayerPanel.ts`:
- Around line 1015-1022: renderAllianceExpiry() is still rendered when isReplay
is true, causing stale expiry text to persist because replay doesn't update the
live-only value; stop rendering or clear the underlying state when in replay:
update the template where renderAllianceExpiry() is used (near the current
conditional around renderActions) so it is not called when isReplay is true, and
additionally ensure show() and/or tick() detect entering replay mode and clear
the alliance-expiry state used by renderAllianceExpiry() (or set it to an
empty/undefined value) to be extra safe; reference renderAllianceExpiry(),
renderActions(), show(), and tick() when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 323978f8-bb58-48ca-92b2-6972e512069b
📒 Files selected for processing (3)
src/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/RadialMenuElements.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
|
@FloPinguin I tested it localy and needed to apply some changes. I added the features and screenshots. |
|
Thank you, I think it would make sense to just show the info panel on right click, skip the info-only-radial? Yeah please check if rabbit is correct, sometimes it has good ideas / finds bugs, you can reply to its comments |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/graphics/layers/MainRadialMenu.ts`:
- Around line 179-184: The early return when myPlayer is null leaves the radial
open; before returning from MainRadialMenu's logic, explicitly close the menu
(e.g. call this.close() or this.hide() on the radial instance) so
spectator/read-only mode is enforced; update the code around the myPlayer null
check (the block that uses this.clickedTile and calls this.updatePlayerActions)
to first close the radial then return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6eec8e2-5cd9-440b-bbdc-ff5a7ac0cc63
📒 Files selected for processing (3)
src/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/PlayerPanel.tssrc/core/game/GameView.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/PlayerPanel.ts
| const myPlayer = this.game.myPlayer(); | ||
| if (myPlayer === null) return; | ||
| const tile = this.clickedTile; | ||
| myPlayer.actions(tile).then((actions) => { | ||
| this.updatePlayerActions(myPlayer, actions, tile); | ||
| }); |
There was a problem hiding this comment.
Close stale radial when viewer becomes spectator.
At Line 180, early return keeps an already-open radial menu visible after spectator transition. Close it before returning so read-only mode is enforced consistently.
Proposed fix
async tick() {
if (!this.radialMenu.isMenuVisible() || this.clickedTile === null) return;
const myPlayer = this.game.myPlayer();
- if (myPlayer === null) return;
+ if (myPlayer === null || this.game.isSpectator()) {
+ this.closeMenu();
+ return;
+ }
const tile = this.clickedTile;
myPlayer.actions(tile).then((actions) => {
this.updatePlayerActions(myPlayer, actions, tile);
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const myPlayer = this.game.myPlayer(); | |
| if (myPlayer === null) return; | |
| const tile = this.clickedTile; | |
| myPlayer.actions(tile).then((actions) => { | |
| this.updatePlayerActions(myPlayer, actions, tile); | |
| }); | |
| const myPlayer = this.game.myPlayer(); | |
| if (myPlayer === null || this.game.isSpectator()) { | |
| this.closeMenu(); | |
| return; | |
| } | |
| const tile = this.clickedTile; | |
| myPlayer.actions(tile).then((actions) => { | |
| this.updatePlayerActions(myPlayer, actions, tile); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/graphics/layers/MainRadialMenu.ts` around lines 179 - 184, The
early return when myPlayer is null leaves the radial open; before returning from
MainRadialMenu's logic, explicitly close the menu (e.g. call this.close() or
this.hide() on the radial instance) so spectator/read-only mode is enforced;
update the code around the myPlayer null check (the block that uses
this.clickedTile and calls this.updatePlayerActions) to first close the radial
then return.
|
@FloPinguin I have implemented your and evans suggestions. |
|
@evanpelle follow up because 4 days without answer |
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
Bot
added
the
Stale
FloPinguin
added
will not stale
|
Added "will not stale" because the PR is waiting for an evan review |
686add7 to
5abc443
Compare
Show player info (resources, alliances, stats) when clicking on a player during replay playback. Action buttons are hidden as they are not applicable in replay mode.
The previous commit made PlayerPanel render when myPlayer is null, but the panel is only opened from the right-click radial menu, which itself short-circuited on myPlayer === null. So the feature was unreachable in replays. - MainRadialMenu: in replay mode, skip the server actions() lookup and open the radial with empty PlayerActions on player tiles only - RadialMenuElements: widen MenuElementParams.myPlayer to PlayerView | null; root submenu returns [info] only when myPlayer is null; guard deleteUnit cooldown/disabled against null Also applies prettier to PlayerPanel.ts (CI was failing on it).
evanpelle pointed out that being dead is basically the same as watching a replay (and so is pre-spawn). Added game.isSpectator() to cover all three and replaced the isReplay checks in PlayerPanel. FloPinguin suggested skipping the info-only radial and just opening the PlayerPanel directly on right-click. That let us revert all the null-threading in RadialMenuElements. Also fixed the stale clickedTile coderabbit flagged, and gated the send-resource and moderation modals so they can't render for spectators.
5abc443 to
03cd573
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
4 participants
Description:
Makes the PlayerInfoPanel accessible during replay playback, as suggested by @FloPinguin.
Previously the panel returned empty immediately when
myPlayer()was null (which is always the case in replays since the viewer is not an active participant). The panel now renders in a read-only mode showing allinformational content: player name, flag, resources, alliances, betrayal count, and trading status.
The right-click radial menu was also gated on
myPlayer !== null, so it never opened in replays. In replay mode the radial now opens with only the info wedge enabled when right-clicking on a player tile, which is theentry point to the panel.
Action buttons that require sending intents to the server are hidden in replay mode since they are not applicable:
Read-only content (resources, alliances, stats) remains fully visible, which was the main motivation for this feature: being able to review alliances during a replay.
Screenshots
Radial menu only shows info option.
Displays all read-only content.
Please complete the following:
Tested locally with
npm run dev:prodagainst a real prod replay (/w1/game/tvBd246s?live).Please put your Discord username so you can be contacted if a bug or regression is found:
sxndrexe